sequenceDiagram
  participant Scheduler
  participant GroupFIFO
  participant WorkerThreads
  Scheduler->>GroupFIFO: inspect group FIFO head
  Scheduler->>WorkerThreads: reserve all target workers
  WorkerThreads-->>Scheduler: report worker availability
  Scheduler->>GroupFIFO: pop group when every target is idle
  Scheduler->>WorkerThreads: dispatch group and clear reservations
  Scheduler->>WorkerThreads: dispatch singles to remaining workers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review — approve with nits

Reviewed 321954ba...be82742b (6 files, +93/−45; Core is 13 lines). The fix does exactly what #1551 specified and I could not break it. Notes below are ordered by what I think is worth acting on.

What I verified rather than assumed

• The regression test is a real barrier. I rebuilt the PR's test_scheduler.cpp against the pre-fix scheduler.{cpp,h} in a scratch build dir. It fails 5/5 on exactly the two intended assertions:

  test_scheduler.cpp:797: Failure  Value of: single_a_overtook       Actual: true   Expected: false
  test_scheduler.cpp:810: Failure  Value of: group_preceded_singles  Actual: false  Expected: true
  

  LaunchableGroupPrecedesConflictingSingles still passes pre-fix, so the new test is not a duplicate of it. Post-fix all 3 scheduler tests pass, and ctest --repeat until-fail:200 is 200/200 clean in 37.8 s — not flaky on this box.

• Reservation lifetime is sound. dispatch_ready() is the only caller of both helpers and runs solely on sched_thread_ (scheduler.cpp:201,219), so the set needs no synchronization; the unconditional clear() at function entry means a reservation can never leak into a later round, including across stop()/start().

• #include <algorithm> removal is safe — std::find was its only use in the file.

• pto_isa.pin (83d0131…) untouched, no pto-isa include changes. No action.

Should fix

1. "Unrelated worker queues progress" has zero test coverage.

That claim is in the PR body and now in three docs, but GroupSchedulerFixture has exactly two NEXT_LEVEL workers (test_scheduler.cpp:640-641,657-658) and the new test makes both of them group targets. A regression that over-reserves — say reserving every worker id instead of the head group's — would pass the entire suite. Adding a third worker and asserting that a single on the non-target worker still dispatches while the group is blocked is the assertion that separates "reserves the right workers" from "stalls the scheduler".

2. reserved_next_level_worker_ids_ carries two load-bearing invariants and no comment.

scheduler.h:101 declares it bare. Two non-obvious facts hold it up:

• The set is meaningful only between dispatch_next_level_group() and dispatch_next_level_singles() within one dispatch_ready(). Reorder those calls, or add a second caller of the singles pass, and reserved workers are silently starved forever.
• The clear() at :430 is not redundant with the one at :395. It is what stops a second group in the same round, sharing a target with the first, from hitting insert().second == false and throwing "duplicate target worker" — which this file's own comment (:344-350) notes is fatal to the whole worker tree via std::terminate, not a per-task failure. A future reader is quite likely to delete :430 as dead.

No test covers that path either — zero tests in the file submit more than one group.

Optionally, keeping the duplicate check on the local workers vector would decouple reservation from malformed-group detection and make :430 obviously non-load-bearing:

if (std::find(workers.begin(), workers.end(), worker) != workers.end()) {
    throw std::runtime_error("Scheduler::dispatch_next_level_group: duplicate target worker");
}
reserved_next_level_worker_ids_.insert(worker_id);

(The check is unreachable either way — Orchestrator::validate_worker_eligibility already rejects duplicate targets at submit time, orchestrator.cpp:622.)

Consider

3. The if (!group_preceded_singles) branch (test_scheduler.cpp:812-824) is dead under correct behavior. It doesn't mask the failure (EXPECT_TRUE already records it), but it duplicates the assertions and reads as though both orderings were acceptable.

4. single_a_overtook is worker_a.dispatched_count() > 1, which is equally true if the group partially dispatched to the reserved-idle worker. The old test asserted no-partial-dispatch explicitly (EXPECT_EQ(worker_b.dispatched_count(), 0)); that now survives only implicitly under a misleading name. Partial group dispatch is a distinct and more serious failure.

5. Pre-existing, out of scope, flagging because this PR is why people will look at that loop: Scheduler::run()'s cv predicate includes !ready_next_level_queues->empty(), and NextLevelReadyQueues::empty() returns false whenever group_queue_ is non-empty (types.cpp:127-133). A blocked group is deliberately left at the head, so the predicate is permanently satisfied — wait() returns immediately every iteration and the scheduler thread spins at 100% CPU until some worker completes. Not introduced here, and this PR shortens the window if anything.

The one thing that needs a decision

This PR adds a new invariant to docs/scheduler.md:189 and docs/directed-next-level-scheduling.md:66:

A running task does not wait for a not-yet-dispatched task at the same scheduler level. Work that requires concurrent placement uses the group API.

Documenting it is the right call — it is the precondition the fix now depends on. The problem is that the repo currently violates it in 10 places.

Why the invariant is load-bearing

TWAIT is an unbounded device-side spin. From examples/workers/l3/allreduce/kernels/aiv/allreduce_onephase_kernel.cpp:112-126:

// Phase 2: device barrier — each rank notifies every peer that its
// stage-in is visible, then waits until every peer has notified us.
for (int peer = 0; peer < nranks; ++peer) { ... TNOTIFY(sig, 1, AtomicAdd); }
for (int peer = 0; peer < nranks; ++peer) { ... TWAIT(sig, 1, GE); }

A collective rank does not return until every peer rank has been dispatched. So "all N ranks co-resident" is a hard precondition — which is the definition of the group API's all-or-nothing dispatch. Yet every collective in the tree is submitted as N independent submit_next_level() calls.

How that deadlocks after this PR

Two workers, a 2-rank collective C submitted as singles, a group G targeting {w0, w1}:

t1  C_rank0 READY; G still PENDING, so no reservation
    → singles: w0 idle → C_rank0 dispatched
    ★ w0 enters TWAIT waiting for rank1. w0->idle() is false from here on, forever

t2  C_rank1 still PENDING (its own producer hasn't finished)

t3  G becomes READY, targets {w0, w1}
    → group: w0 busy → all_workers_idle = false → reserve {w0, w1}   ← new in this PR

t4  C_rank1 becomes READY, lands in w1's single FIFO
    → group: w0 still in TWAIT → reservation {w0, w1} re-derived
    → singles: w1 is reserved → continue                              ← C_rank1 never dispatched

t5  C_rank1 not dispatched → w0's TWAIT never returns → w0 never idle
    → G never launches → reservation never lifts → C_rank1 not dispatched

t1 is the hinge: rank0 must get on-device before G becomes READY. Once it is in TWAIT, w0 is permanently non-idle, so the group can never assemble and the reservation is permanent. If rank0 becomes READY after G, it is simply withheld, w0 stays idle, G launches normally and everything drains — so this is timing-dependent, not deterministic.

Before this PR the same sequence made progress: at t4 w1 was idle and unreserved, C_rank1 dispatched, the barrier converged, w0 freed, G launched.

It is not reachable today

The shape needs a group and a TWAIT single coexisting in one DAG, and nothing in the tree does that:

So this PR cannot deadlock anything in-tree. I'm not asking to hold the merge on it. But three fairly natural next steps introduce it:

1. Fusing dual_domain_overlap's three worker.run calls into one DAG. The example is literally named overlap but executes serially — making it actually overlap is the obvious follow-up, and its affine group targets are exactly the domain members.
2. Adding an epilogue group to ffn_tp_parallel, which already has the per-rank dependency skew that produces t1/t2.
3. Pipelined multi-round orchestration (feat: support pipelined orchestration and scheduling across tokens and requests #1379) — round N's group coexisting with round N+1's collective singles. This is the motivation [Bug] READY NEXT_LEVEL group can be overtaken indefinitely by later singles (no Tier-0 equivalent at L3) #1551 itself cites.

Why it would be nasty to debug

The scene at hang time: reserved workers sitting idle (resources free, nothing running), one worker at 100% AIV spinning in TWAIT (looks like it's computing), the group slot stuck at READY forever, and eventually HandleTaskTimeout killing aicpu-sd → surfaces as a plain 507018. No allocator/dep deadlock detector fires, because it isn't a capacity problem — and per the triage table in .claude/rules/running-onboard.md, "only HandleTaskTimeout fired" routes you toward "long or stalled op", i.e. away from the real cause. The actual cause is one unordered_set on the host scheduler and the device log never mentions it.

Proposed follow-ups

A. Move collectives onto the group API

Criterion: kernel contains TWAIT ⇒ must be a group; no TWAIT ⇒ stays a single.

The edit is mechanical — accumulate in the loop, submit once after:

        args_list = []
        for i in range(nranks):
            chip_args = TaskArgs()
            ...
            args_list.append(chip_args)
        orch.submit_next_level_group(chip, args_list, config, workers=list(range(nranks)))

Two things worth noting, both in favour:

• It turns a device spin into a scheduler wait. Today in ffn_tp_parallel, rank i's allreduce is dispatched the moment rank i's matmul finishes, then burns AIV in TWAIT waiting for the others. As a group it stays READY-pending until every rank's host_partial is produced (infer_deps consumes the whole args_list), leaving the worker free meanwhile.
• It removes a hidden dependence on FIFO ordering. With two overlapping domains, correctness currently rests on each chip's FIFO order being mutually consistent; an inversion is a wait cycle. All-or-nothing dispatch removes the precondition structurally.

This should be its own PR — 10 sites, touches the ST suite, needs onboard validation. Not for #1565.

B. A stall watchdog on the reservation

Time alone can't separate deadlock from a legitimately long op. The discriminator is structural: a reserved worker that is IDLE and has a non-empty single FIFO. All-targets-busy is an ordinary long-op wait and must not warn; an idle reserved target with queued work means the scheduler is actively withholding runnable work from an idle resource — exactly state t4 above. Even when it isn't a deadlock, that state is worth reporting.

Sketch (host-side, so codestyle.md §7 doesn't bind, but the report is edge-triggered anyway — the dispatch loop runs far too often to log per round):

// Well below the 45s op-execute timeout, so the report always precedes the
// 507018 that would otherwise be the only symptom.
constexpr auto kReservationStallWarnAfter = std::chrono::seconds(5);

void Scheduler::check_reservation_stall(TaskSlot group_slot) {
    const auto now = std::chrono::steady_clock::now();
    if (reserved_next_level_worker_ids_ != reported_reservation_) {   // set changed → progress
        reported_reservation_ = reserved_next_level_worker_ids_;
        reservation_since_ = now;
        reservation_stall_reported_ = false;
        return;
    }
    if (reservation_stall_reported_) return;
    if (now - reservation_since_ < kReservationStallWarnAfter) return;

    bool withholding = false;   // an idle reserved target with queued singles
    for (int32_t id : cfg_.ready_next_level_queues->worker_ids()) { ... }
    if (!withholding) return;

    reservation_stall_reported_ = true;
    std::fprintf(stderr, "[simpler][scheduler] NEXT_LEVEL group slot=%d has held its target "
        "reservation for >5s without launching. targets:%s\n"
        "  If a task on a BUSY target is waiting for a peer task queued on a reserved target, "
        "this will never resolve. The usual cause is a collective submitted as N separate "
        "submit_next_level() calls instead of one submit_next_level_group().\n", ...);
}

Hook it on the blocked return in dispatch_next_level_group():

        if (!all_workers_idle) {
            check_reservation_stall(slot);
            return;
        }

Needs a small NextLevelReadyQueues::single_empty(worker_id) accessor; ReadyQueue::empty() already exists. No reset code needed on launch — clear() makes the next round's set differ, which re-arms the watchdog. It must be a log, not a throw: an exception on sched_thread_ is std::terminate for the whole tree.

Testable in the existing fixture by simply never calling complete() on the mock worker holding the busy target.

I'd argue against making it self-healing (releasing the reservation on timeout). That contradicts #1551's "waits and retries rather than yielding its share back", brings the starvation bug back in intermittent form, and would permanently mask the submission bug that caused it. Better to hang, but hang legibly.

Suggested split

• This PR: Should-fix 1 and 2 (third worker in the fixture, invariant comment). Both are small and in scope.
• This PR or immediately after: follow-up B, the stall watchdog — it's the diagnostic for the failure mode this PR introduces, and the two together are the complete change. If split, please leave an issue link here.
• Separate PR: follow-up A, the collective → group migration.

Nice work on this one — the core change is 13 lines, the docs landed in the same commit across all three affected files, and documenting the new precondition instead of leaving it latent is what let the rest of this analysis happen at all.

_{Cross-checked with codex review and gemini; both returned no findings. Codex independently reproduced the pre-fix failure and ran the 200× stress loop. Gemini's "no missing tests" conclusion was dropped — it read the new test's singles as landing on unrelated workers, but they're on the group's own targets (the fixture only has two workers), which is Should-fix 1.}